security: fix prompt injection, path traversal, SSRF, and unsafe YAML parsing#16
Open
iliassjabali wants to merge 3 commits intomainfrom
Open
security: fix prompt injection, path traversal, SSRF, and unsafe YAML parsing#16iliassjabali wants to merge 3 commits intomainfrom
iliassjabali wants to merge 3 commits intomainfrom
Conversation
… issues
SEC-01/02 — Prompt injection via source files and YAML repair content
context-builder.ts: source files and manifest JSON are now wrapped in
<context_manifest> / <context_file> XML tags instead of bare markdown
fences. Prevents a scanned source file from injecting LLM instructions.
index.ts (repairYaml): YAML content wrapped in <yaml_content> XML tags;
truncated to 64 KB. System prompt instructs Claude to treat tagged content
as data only.
skills/guidelines.md: security preamble added — Claude must treat all
<context_*> content as data, never as instructions.
SEC-03 — Path traversal in $file: tool module references
context-builder.ts extractFileRefs(): resolves each $file: path and checks
it starts with manifestDir + sep. Paths escaping the directory are silently
skipped (prevents $file:../../etc/passwd from being read and sent to Claude).
BUG-04 — js-yaml unsafe schema allows !!js/function execution
sdk/src/loader/index.ts: yaml.load now uses { schema: yaml.JSON_SCHEMA }.
The default schema supports !!js/regexp and !!js/function which can execute
code when parsing a malicious agent.yaml.
SEC-08 — SSRF: RFC 1918 private addresses not blocked in service health checks
service.check.ts classifyHost(): added blocking for 10.0.0.0/8,
172.16.0.0/12, and 192.168.0.0/16. Previously only loopback and link-local
were blocked; a manifest could reference redis://10.x.x.x to probe internal
cluster services.
BUG-05 — Dataset path traversal in evaluate.ts
Added bounds-check after resolving dataset path. If absPath does not start
with manifestDir + sep, the command exits 1.
BUG-02 — Biased shuffle in evaluate.ts --sample-size
Replaced Array.sort(() => Math.random()-0.5) with Fisher-Yates. The sort
idiom violates transitivity and produces a non-uniform distribution.
QUAL-08 — OPA parseCommaSeparatedHeader discards all but first array value
opa-client.ts: Array.isArray(value) ? value[0] : value replaced with
value.join(',') so all HTTP header instances are included.
Tests: updated claude-adapter.test.ts for new XML context format; added
path-traversal guard test; added RFC 1918 SSRF tests to service-check.test.ts
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Security-focused hardening across the CLI, SDK, sidecar, and Claude adapter to reduce prompt-injection risk, prevent path traversal reads, tighten SSRF protections, and make YAML parsing safer when loading untrusted manifests.
Changes:
- Wrap Claude context inputs (manifest + context files) with XML-style delimiters and add
$file:traversal bounds checks. - Make manifest YAML loading safer by restricting
js-yamltoJSON_SCHEMA. - Expand service health-check SSRF blocking to include RFC1918 private IPv4 ranges; improve correctness in CLI evaluation (dataset path guard + unbiased sampling shuffle) and sidecar OPA header parsing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/adapter-claude/src/context-builder.ts | Adds XML-style wrappers and $file: bounds checks when building Claude context |
| packages/adapter-claude/src/skills/guidelines.md | Intended to add system-prompt hardening guidance for untrusted content handling |
| packages/adapter-claude/src/index.ts | Intended to harden repairYaml prompt construction against YAML prompt-injection |
| packages/sdk/src/loader/index.ts | Uses yaml.JSON_SCHEMA to avoid unsafe YAML types |
| packages/sdk/src/health/checks/service.check.ts | Blocks RFC1918 IP ranges in host classification to reduce SSRF risk |
| packages/cli/src/commands/evaluate.ts | Adds dataset path bounds check + replaces biased shuffle with Fisher–Yates |
| packages/sidecar/src/control-plane/opa-client.ts | Preserves multiple header instances by joining before comma-splitting |
| packages/adapter-claude/src/tests/claude-adapter.test.ts | Updates tests for new context format + adds traversal guard test |
| packages/sdk/src/tests/service-check.test.ts | Adds RFC1918 SSRF coverage and host:port parsing expectations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ements context-builder.ts: - Add missing security preamble to guidelines.md (was only on feat/claude-subscription-auth branch) instructing Claude to treat <context_*> XML tags as data only, never as instructions - Escape XML attribute values (path, lang) with a minimal XML escaper so file paths containing ", <, & etc. cannot produce malformed tags or inject content outside the attribute - Sanitize file content to encode </context_file> as <\/context_file> so a source file cannot break out of its wrapper and inject instructions - Add lstatSync check in extractFileRefs() to reject symlinks before reading; a symlink inside baseDir pointing outside would have bypassed the realpath prefix guard evaluate.ts: - Replace the lexical-only dataset path guard with a two-step check: the existing resolve()-based prefix test (fast, catches ..) followed by realpathSync on both the candidate and the base directory so symlinks inside manifestDir that point outside cannot bypass the guard service.check.ts: - Use net.isIP() to distinguish IP literals from hostnames before applying RFC 1918 / loopback / link-local prefix checks; hostnames like '10.example.com' no longer produce a false positive - Document DNS-rebinding limitation (out of scope for sync health checks) Tests: - service-check.test.ts: add two false-positive hostname tests; fix node:net mock to use importOriginal so net.isIP is available - claude-adapter.test.ts: add symlink-escape test, XML attribute escaping test, and </context_file> content breakout test - evaluate.test.ts: add realpathSync to node:fs mock (returns path unchanged)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes six independent security and correctness issues identified during a codebase audit, plus four follow-up improvements from Copilot's review of the initial commit.
Changes
SEC-01/02 — Prompt injection via source files and YAML repair content
context-builder.ts: Source files and the manifest JSON are wrapped in<context_manifest>/<context_file>XML tags.skills/guidelines.md: Security preamble instructs Claude to treat all content inside<context_manifest>and<context_file>tags as data only, never as instructions — regardless of what the file content says.Attribute escaping: File path and language attributes are XML-escaped (
&,",<,>) so paths containing special characters cannot produce malformed tags or break out of the attribute value.Content sanitization:
</context_file>is encoded to<\/context_file>inside file content, so a source file that contains the exact closing tag string cannot terminate its wrapper early and inject text outside the boundary.index.ts(repairYaml): Raw YAML content wrapped in<yaml_content>/<validation_errors>tags; system prompt hardened; content truncated to 64 KB.SEC-03 — Path traversal + symlink escape in
$file:tool module referencescontext-builder.tsextractFileRefs():$file:path and checks it starts withmanifestDir + sep.lstatSyncis called beforereadFileSync; any entry whose stat reportsisSymbolicLink()is rejected. A symlink insidemanifestDirpointing to/etc/passwdwill no longer be read.BUG-04 —
js-yamlunsafe schema allows!!js/functionexecutionsdk/src/loader/index.ts:yaml.load(raw, { schema: yaml.JSON_SCHEMA }). The default schema supports!!js/functionand other unsafe custom YAML types.SEC-08 — SSRF: RFC 1918 private addresses reachable from service health checks
service.check.tsclassifyHost():net.isIP()to determine whether the host value is an IP literal before applying range checks. Hostnames like10.example.comno longer produce a false positive (Copilot review finding).10.0.0.0/8,172.16.0.0/12,192.168.0.0/16IP literals.BUG-05 — Dataset path traversal + symlink escape in
evaluate.tsTwo-step guard:
resolve()-normalized path (catches../..).realpathSyncon both the candidate path and the base directory, then re-check — a symlink insidemanifestDirpointing outside can no longer bypass the string-prefix guard.BUG-02 — Biased shuffle in
evaluate.ts --sample-sizeFisher-Yates replaces
Array.sort(() => Math.random() - 0.5).QUAL-08 — OPA
parseCommaSeparatedHeaderdiscards all but first array valuevalue.join(',')replacesvalue[0].Tests
adapter-claude/__tests__/claude-adapter.test.ts</context_file>breakout test; symlink-escape test forextractFileRefs; traversal guard testsdk/__tests__/service-check.test.ts10.example.com,192.168.example.com);node:netmock updated to forwardnet.isIPviaimportOriginalcli/__tests__/evaluate.test.tsrealpathSyncadded tonode:fsmockAll 1031 tests pass across all packages.
Files changed
packages/adapter-claude/src/context-builder.tslstatSyncsymlink guard +$file:traversal guardpackages/adapter-claude/src/skills/guidelines.md<context_*>content as data onlypackages/adapter-claude/src/index.tsrepairYamlXML tags + 64 KB truncationpackages/sdk/src/loader/index.tsyaml.JSON_SCHEMApackages/sdk/src/health/checks/service.check.tsnet.isIP()guard + RFC 1918 blocklistpackages/cli/src/commands/evaluate.tspackages/sidecar/src/control-plane/opa-client.tspackages/adapter-claude/src/__tests__/claude-adapter.test.tspackages/sdk/src/__tests__/service-check.test.tsimportOriginalmock fixpackages/cli/src/__tests__/evaluate.test.tsrealpathSyncin fs mock